-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Bugfix: enums from external fragments were not generated along with operations #10565
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master-next
Are you sure you want to change the base?
Bugfix: enums from external fragments were not generated along with operations #10565
Conversation
🦋 Changeset detectedLatest commit: 3839b02 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…config-extractAllFieldsToTypesCompact
| const documentWithExternalFragments: DocumentNode = { | ||
| ...documentNode, | ||
| definitions: [...documentNode.definitions, ...(config.externalFragments || []).map(f => f.node)], | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the functionality in the above statement used to create allFragments could be used for our fragments here somehow? 🤔
It seems to do a similar function of collecting fragments, either from documents or from config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call!
Check the new code - I combined it with allFragments.
dev-test-alpha/.gitignore
Outdated
| @@ -0,0 +1,2 @@ | |||
| **/__generated__/** | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel we should unignore the generation 🙂
This could be a good test that can be kept as snapshot test, and an advanced use case of codegen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, generated files added!
| @@ -0,0 +1,106 @@ | |||
| #!/usr/bin/env ts-node | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created the dev-test-alpha just as a temporary package to demonstrate our company issues.
Do you think it would be worth keeping it?
Then:
- I'll need to add some tests which would get invoked when global tests are running.
- We need to come up with some better name than
dev-test-alpha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think so! I find this test is very valuable, as it can be used to catch unexpected bugs, at least in the initial stage after rolling out.
I feel we might be able to simplify a few things in the setup. And worst case, if its usefulness is overshadowed by maintenance, we can decide to drop it (or further simplify) later 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I renamed dev-test-alpha to dev-test-apollo-tooling - to give a more descriptive name to what this integration test it about. I also added another (hopefully last) fix - so you can run yarn install && yarn build from the root directory: graphql-code-generator and - it should work.
| // @generated | ||
| // This file was automatically generated and should not be edited. | ||
|
|
||
| import type * as Types from '../../graphql-code-generator/dev-test-alpha/src/__generated__/globalTypes'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we already worked on removing this line in typescript-operations plugin.
But it probably comes from near-operation-file preset.
Shall we ignore it until we do the next version of near-operation-file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a research and - yes, it comes from near-operation-file.
|
|
||
| type Exact<T extends { [key: string]: unknown }> = { [K in keyof T]: T[K] }; | ||
| export type Incremental<T> = T | { [P in keyof T]?: P extends ' $fragmentName' | '__typename' ? T[P] : never }; | ||
| export enum UserManagerRoleType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the recent fix - it adds the enum in the correct file.
…config-extractAllFieldsToTypesCompact
| * @see https://github.com/apollographql/apollo-tooling/issues/2053 | ||
| * */ | ||
| const GRAPHQL_CODEGEN_CONFIG = { | ||
| skipTypename: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This config doesn't exist anymore 🙂
| const GRAPHQL_CODEGEN_CONFIG = { | ||
| skipTypename: false, | ||
| useTypeImports: true, | ||
| preResolveTypes: true, // Simplifies the generated types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
preResolveTypes: true is the only option now, and it's been removed entirely.
So we can omit it (when we are ready to merge)
|
|
||
| import type * as Types from '../unused'; | ||
|
|
||
| type Exact<T extends { [key: string]: unknown }> = { [K in keyof T]: T[K] }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Context/braindump, not a comment on this PR)
I made a TODO about Exact being generated but not used in files with only fragments in the feature PR. Some users' tsconfig.json may have strict types so it'd fail tsc.
I believe we are generating Exact all the time, but we should only do it if operations have been found in a document. I can take a look into that in a different PR.
| "typescript": "5.5.4", | ||
| "eslint-plugin-promise": "7.1.0" | ||
| "eslint-plugin-promise": "7.1.0", | ||
| "graphql": "16.9.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think resolving a particular version of graphql is usually done in CI, when we test multiple versions of GraphQL in different actions.
Do we need this for this PR? 🙂
Description
This PR includes the fix to the following issue: enums from external fragment were not copied to the generated file along with operations.
Additionally, an integrated test is added:
dev-test-apollo-toolingThat mimics the Apollo-tooling setup.
Testing